Fix user-agent register timing in C++ and Unity SDK #1217

chkuang-g · 2023-02-22T20:01:44Z

Description

Provide details of the change, and generalize the change in the PR title above.

Patch user-agent register timing in C++ and add utility functions for Unity SDK to register BEFORE C++ App is created.

Several notes:

The original use of util::GetJNIEnvFromApp() is not reliable because it only works AFTER the first app is added.
The new CallAfterEnsureMethodsCached() functions is primarily for Unity SDK to register BEFORE C++ app is created, which is when the Java classes and methods are cached originally.

Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Integration test. Manually E2E test for Android and iOS Auth testapp.

Type of Change

Place an x the applicable box:

Bug fix. Add the issue # below if applicable.
New feature. A non-breaking change which adds functionality.
Other, such as a build process or documentation change.

Notes

Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
Read the contribution guidelines CONTRIBUTING.md.
Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

Change to register user-agents before platform App creation so that the new user-agents can be captured and logged at iOS/Android level.

This utility function is used internally to ensure the callback is triggered when all necessary Java classes and methods are cached.

github-actions · 2023-02-24T01:48:18Z

❌ Integration test FAILED

Requested by @chkuang-g on commit 50828a9
Last updated: Mon Mar 6 18:04 PST 2023
View integration test log & download artifacts

Failures	Configs
analytics	[TEST] [FAILURE] [Android] [All 3 os] [1/2 android_device: android_target] (1 failed tests) FirebaseAnalyticsTest.TestGetSessionID
firestore	[TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/2 android_device: android_target] (1 failed tests) ValidationTest.FieldNamesMustNotBeEmpty
messaging	[TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: android_target] (1 failed tests) CRASH/TIMEOUT

Add flaky tests to go/fpl-cpp-flake-tracker

AlmostMatt

This generally looks good to me. I just added a few minor comments.

app/src/app_common.cc

app/src/util_android.h

app/src/include/firebase/app.h

AlmostMatt · 2023-02-27T21:32:35Z

app/src/util_android.h

+// Make sure the Java classes and methods are cached before triggering the
+// the callback. Can be slow if this is called BEFORE any Firebase App is
+// created.
+void CallAfterEnsureMethodsCached(JNIEnv* env, jobject activity,


It's a little bit strange to me that this is implemented in app_android.cc and the header is util_android.h.

I think the implementation makes sense to be in app_android.cc as it calls CacheMethods and I guess there is not currently an app_android.h file, so I suppose this is fine as is.

It is.

I'm not sure if it is worth adding a app_android.h just for this single function. And unfortunately the implementation has to live in app_android.cpp so that it can access to all the JNI caching and releasing functions.

Why isn't the implementation in util_android, if it's not a method of App or related types like AppOptions?

CallAfterEnsureMethodsCached() is an utility by nature but requires CacheMethods() and ReleaseClasses() which is only available in app_android.cpp.

https://github.com/firebase/firebase-cpp-sdk/blob/main/app/src/app_android.cc

I can think of some alternative solutions

Add app_android.h and declare CallAfterEnsureMethodsCached() in app_android.h

Add app_android.h and declare CacheMethods() and ReleaseClasses() so that they can be linked to util_android.cpp. Then move CallAfterEnsureMethodsCached() to util_android.cpp.

Move declaration and impl of CacheMethods() and ReleaseClasses() to util_android.

Do you see a risk to implement a function declare in util_android.h but implemented in app_android.cpp?

Added app_android.h

🍞 Dismissed stale approval on external PR.

The desktop test fail because the user-agent does not get registered again after the app is deleted an created again. Move the boolean to LibraryRegistry instance so that it can reset when the app is deleted.

🍞 Dismissed stale approval on external PR.

jonsimantov

This seems mostly fine except for a few nits.

jonsimantov · 2023-03-04T09:01:09Z

app/src/app_common.cc

@@ -190,7 +190,7 @@ struct AppData {
 // Tracks library registrations.
 class LibraryRegistry {
 private:
-  LibraryRegistry() {}
+  LibraryRegistry() : is_common_libraries_registered(false) {}


nit: what about naming this, and the accessors, "Library" instead of "Libraries", as IsCommonLibraryRegistered is more correct?

jonsimantov · 2023-03-04T09:02:25Z

app/src/app_ios.mm

@@ -277,7 +281,7 @@ static void PlatformOptionsToAppOptions(FIROptions* platform_options, AppOptions

 App* App::GetInstance(const char* name) { return app_common::FindAppByName(name); }

-void App::RegisterLibrary(const char* library, const char* version) {
+void App::RegisterLibrary(const char* library, const char* version, void* platform_resource) {


If it's not used on this platform, do the /* platform_resource */ thing.

Done. Also changed for desktop and stub

jonsimantov · 2023-03-04T09:04:39Z

app/src/util_android.h

+// Make sure the Java classes and methods are cached before triggering the
+// the callback. Can be slow if this is called BEFORE any Firebase App is
+// created.
+void CallAfterEnsureMethodsCached(JNIEnv* env, jobject activity,


Why isn't the implementation in util_android, if it's not a method of App or related types like AppOptions?

- Rename `IsCommonLibrariesRegistered` to `IsCommonLibraryRegistered`, and similar changes. - Change function parameter `/* platform_resource */` for platforms which are not using this parameter - Move `CallAfterEnsureMethodsCached()` declaration to `app_android.h`

🍞 Dismissed stale approval on external PR.

app/src/app_android.h

chkuang-g

Change applied.

chkuang-g added the skip-release-notes Skip release notes check label Feb 22, 2023

chkuang-g force-pushed the patch/user-agent-timing branch 4 times, most recently from 73d3d33 to 9177497 Compare February 24, 2023 01:45

chkuang-g added 2 commits February 23, 2023 17:47

Change user-agent register timing in C++

99994e6

Change to register user-agents before platform App creation so that the new user-agents can be captured and logged at iOS/Android level.

Add Android utility CallAfterEnsureMethodsCached

06bd6a2

This utility function is used internally to ensure the callback is triggered when all necessary Java classes and methods are cached.

chkuang-g force-pushed the patch/user-agent-timing branch from 9177497 to 06bd6a2 Compare February 24, 2023 01:47

chkuang-g added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label Feb 24, 2023

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Feb 24, 2023

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Feb 24, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Feb 24, 2023

chkuang-g requested review from jonsimantov and AlmostMatt February 24, 2023 06:35

chkuang-g mentioned this pull request Feb 24, 2023

Fix user-agent register timing in Unity SDK firebase/firebase-unity-sdk#635

Merged

3 tasks

AlmostMatt previously approved these changes Feb 27, 2023

View reviewed changes

chkuang-g requested a review from a-maurice March 1, 2023 19:22

Update according to the review feedback

141c3e1

a-maurice previously approved these changes Mar 3, 2023

View reviewed changes

Merge branch 'main' into patch/user-agent-timing

88c003e

chkuang-g added 2 commits March 3, 2023 18:00

Fix test fail

783bd65

The desktop test fail because the user-agent does not get registered again after the app is deleted an created again. Move the boolean to LibraryRegistry instance so that it can reset when the app is deleted.

rename the function

4e00f82

a-maurice previously approved these changes Mar 4, 2023

View reviewed changes

format code

d6c682b

jonsimantov previously approved these changes Mar 4, 2023

View reviewed changes

fix lint

4aa4f40

chkuang-g commented Mar 6, 2023

View reviewed changes

app/src/app_android.h Show resolved Hide resolved

format_code

3e68401

chkuang-g commented Mar 6, 2023

View reviewed changes

jonsimantov approved these changes Mar 6, 2023

View reviewed changes

chkuang-g merged commit 50828a9 into main Mar 6, 2023

chkuang-g deleted the patch/user-agent-timing branch March 6, 2023 23:26

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: succeeded This PR's integration tests succeeded. labels Mar 6, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Mar 7, 2023

firebase locked and limited conversation to collaborators Apr 6, 2023

Fix user-agent register timing in C++ and Unity SDK #1217

Fix user-agent register timing in C++ and Unity SDK #1217

Uh oh!

Conversation

chkuang-g commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Type of Change

Notes

Uh oh!

github-actions bot commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

AlmostMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsimantov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chkuang-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chkuang-g commented Feb 22, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading